refactor: move address/spent/timestamp indexes into NodeContext - #7547

🔍 Review in progress — actively reviewing now (commit 67a915a)
Stage: Codex precheck starting
ETA: complete ~23:00 UTC (median 20m across 30 recent reviews)
Running 9m · Last checked: 2026-08-05 22:50 UTC

No actionable comments were generated in the recent review. 🎉

Walkthrough

Address, spent, and timestamp indexes are now owned by NodeContext instead of global pointers. Initialization, interruption, shutdown, and RPC access use the node-owned instances. Mempool indexing uses explicit enablement options. Transaction JSON conversion receives a spent-index pointer from its callers.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RPCClient
  participant NodeContext
  participant IndexService
  RPCClient->>NodeContext: request index-backed RPC
  NodeContext->>IndexService: access configured index
  IndexService-->>NodeContext: return synchronization and lookup data
  NodeContext-->>RPCClient: return RPC response

Possibly related PRs

• dashpay/dash#7525: Both PRs modify address, spent, and timestamp index classes, but this PR removes global ownership while that PR refactors constructors.

_{Comment @coderabbitai help to get the list of available commands.}

Bitcoin still keep them as globals:

src/index/txindex.h:extern std::unique_ptr<TxIndex> g_txindex;

Why should we diversify codebases here?

I don't see visible benefits to do it right now. Also bitcoin still hasn't get merged bitcoin#24230 which should make proper solution so I would wait that one get merged first.

one more thing:
index/base -> node/context -> net_processing -> index/txindex -> index/base
is not equivalient to index/base -> node/context -> index/txindex -> index/base

Because in one case node/context is a key element that making circle, and in other case it's net_processing.

The three indexes moved here are Dash-specific — AddressIndex, SpentIndex, and TimestampIndex have no upstream counterpart, so there's no shared code to diverge from in these files; the globals themselves were our invention. The upstream-shared indexes (g_txindex, g_coin_stats_index, the blockfilter registry) are deliberately untouched, so every site a Bitcoin backport can conflict with keeps the exact global pattern upstream uses. The init/RPC hunks touched here are all Dash-only blocks (getaddressbalance, getspentinfo, getblockhashes, …). The benefits aren't speculative either — they're in the diff: txmempool.cpp no longer re-declares extern std::unique_ptr<AddressIndex> g_addressindex; just to use it as an enable flag, TxToJSON stops reading a global, and the indexes become per-context like every other Dash subsystem already is (dmnman, govman, llmq_ctx, active_ctx, and CDSNotificationInterface in #7546). They were the last Dash-owned components still living in globals, and the NodeContext doc comment upstream wrote ("More variables could be added to this struct … to eliminate use of globals and make code more modular and testable") is explicitly inviting this.

On waiting for bitcoin#24230: I don't think it's the right thing to gate on. It's been a draft since Feb 2022 (re-pushed July 2026, CI currently red, long conflict list), and more importantly it isn't an ownership PR — its commits pull sync logic out of the index classes and into node code so indexes can eventually run out-of-process. It never moves g_txindex into NodeContext, so there's nothing here that diverges from it.

On the cycles — fair distinction, agreed: the new entries put node/context directly in the cycle, where the existing one only reaches index/txindex through net_processing. But note what the edges carry. net_processing -> index/txindex is functional coupling (message handling calls g_txindex->HasTx()). The new node/context -> index/* edges carry no calls at all — context.cpp includes those headers solely so the out-of-line ~NodeContext can instantiate the unique_ptr deleters, exactly as it already does for txmempool.h, llmq/context.h, etc. The lint works at module granularity and can't tell a destructor-instantiation include from a real dependency; semantically the new cycles are weaker than the tolerated one.

And this is where 24230 actually cuts the other way: all four entries — the pre-existing txindex one included — share the same root edge, index/base -> node/context, which exists only because BaseIndex::Start() does m_chain->context()->chainman->ActiveChainstate(). The comment above that line in our own tree already says: "m_chainstate member gives indexing code access to node internals. It is removed in followup bitcoin#24230." So the allowlist entries are precisely as temporary as 24230: when it lands and we backport it, all four cycles disappear together. Ownership-in-NodeContext now plus the 24230 backport later composes cleanly; the reverse order just keeps the Dash indexes global in the meantime for no benefit.

(I did look at avoiding the entries outright — unique_ptr<BaseIndex> members still cycle index/base <-> node/context directly and force downcasts in RPC; deleter-erasure/pimpl is worse than three allowlist lines already scheduled for deletion by the thing they'd be waiting on.)

🤖 Posted autonomously by Claude on behalf of pasta.

If the underlying worry is compile time, this cycle is the kind that has none. Compile-time cost from circular dependencies comes from header→header edges: a widely-included header transitively pulling more headers (bigger preprocessed TUs everywhere), or a low-level header depending on a high-level one (wide rebuild cascades when it changes). This PR adds no header→header edges at all: node/context.h gains only three forward declarations, so every TU that includes it preprocesses to the same thing as before, and touching node/context.h rebuilds exactly the same set of TUs as on develop.

The new cycle edges live entirely in .cpp files, where an include affects exactly one TU: node/context.cpp (+3 index headers, for the unique_ptr deleter instantiation), node/mempool_args.cpp (+2, for the DEFAULT_* constants), and rpc/blockchain.cpp (+1). rpc/node.cpp already included all three index headers before this PR. The total incremental-build delta is that editing an index header now rebuilds two additional small TUs (context.cpp, mempool_args.cpp) — and index headers are edited rarely. Nothing that includes node/context.h (init, rpc, interfaces, tests, …) is affected in any way.

That's also the general property of this cycle class: a module-level cycle closed through .cpp-side includes is invisible to the build graph's fan-out; the lint flags it because it can't distinguish include-for-deleter from include-for-use. The header-graph direction stays strictly index/* -> node/context (pre-existing, one TU: base.cpp), never the reverse.

🤖 Posted autonomously by Claude on behalf of pasta.

If the underlying worry is compile time, this cycle is the kind that has none

no, not at all. Every newly introduced exception for circular dependencies firstly shadowing new circular dependencies that are super-set of already existing loop. Secondly, breaking them in the future is always extra efforts to do

Secondly, the main reason is diversification of codebases.

Yes, indeed, g_txindex is bitcoin + dash used but g_timestampindex and 2 others are dash specific.

Though,
they are very much related, implementing the same interface and following the same usages all over code base. What exactly does it improve? Get rid of global variable? Is there any specific case where is any tiniest benefits to out-weight of codebase diversification?

Is there any specific case where is any tiniest benefits to out-weight of codebase diversification?

Three specific ones:

1. Link-time coupling. txmempool.cpp referencing the g_addressindex/g_spentindex symbols means anything that links the mempool needs the index objects linked in too. We've already paid this exact cost once with an index global: 2b2d0a4 had to move CDeterministicMN::ToJson into core_write specifically "to avoid g_txindex dependency for linking". After this PR the mempool carries two bools and links standalone — that extern re-declaration hack in txmempool.cpp is deleted outright.

2. Test isolation and teardown. These indexes hold LevelDB handles. As globals, a unit test that enabled one leaks it (and its mempool side effects) into every subsequent test in the same binary, and any path that skips PrepareShutdown hands destruction to cross-TU static-destructor ordering — the classic shutdown-crash class. As NodeContext members they're constructed and destroyed per-TestingSetup, deterministically, and always before the things they reference. That's what makes in-process unit coverage of the Dash indexes tractable at all; today they're functional-test-only.

3. Consistency with where Dash state already lives. dmnman, govman, mn_sync, llmq_ctx, active_ctx, and now CDSNotificationInterface (refactor: move CDSNotificationInterface into NodeContext #7546) are all NodeContext-owned; the three Dash indexes were the last Dash-owned components still in globals, and their RPCs reaching them through EnsureAnyNodeContext is the same shape every other Dash RPC already uses.

On diversification: every file this touches is Dash-only (index/{address,spent,timestamp}index.*, the Dash RPCs, Dash-only init hunks), while g_txindex/g_coin_stats_index/blockfilter keep the upstream global pattern byte-for-byte — so the conflict surface for future backports is unchanged. And the fact that the Dash indexes implement the same BaseIndex interface is what makes the eventual unification cheap, not what argues for keeping them global: whenever bitcoin#24230 (or a node.indexes-style ownership PR) is merged and backported, the Dash side is already reached through node. and folds in mechanically — and that same backport deletes the index/base -> node/context edge, which removes all four allowlist entries (the pre-existing txindex one included) with zero extra effort. So "breaking them in the future" isn't extra work here; it falls out of the very PR you'd be waiting for.

On shadowing: the lint matches exact chains, so a future cycle passing through any additional module produces a different string and still fails. What the entry does tolerate is someone later adding a real call-dependency along the same edge — which is the identical exposure the existing txindex entry already has. Happy to annotate the three entries with a # remove when bitcoin#24230 is backported marker so the cleanup obligation is recorded in-tree rather than only in this thread.

🤖 Posted autonomously by Claude on behalf of pasta.